feat: support AbortSignal on server-side methods#41141
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
|
||
| async scrollIntoViewIfNeeded(options: channels.ElementHandleScrollIntoViewIfNeededOptions & TimeoutOptions = {}) { | ||
| await this._elementChannel.scrollIntoViewIfNeeded({ ...options, timeout: this._frame._timeout(options) }); | ||
| await this._elementChannel.scrollIntoViewIfNeeded({ ...options, timeout: this._frame._timeout(options), signal: options.signal }); |
There was a problem hiding this comment.
In theory, can we replace timeouts with client-side signals that abort on timeout? Worth an experiment?
There was a problem hiding this comment.
I think we could, yes! I'll try it out.
a782655 to
c7b916c
Compare
Threads an AbortSignal through the client as a required second argument on channel methods (mirroring progress on the dispatcher side), so server-side calls can be cancelled.
c7b916c to
07a7b5c
Compare
|
Rebased onto #41321. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Aborting a channel call now rejects with an AbortError (mirroring Node.js abort-aware operations), preserving the original abort reason as cause instead of rethrowing the raw reason. This guarantees Playwright never throws a non-Error value when aborted with a string reason, in both the in-flight and already-aborted cases.
fe1c16f to
1b21b1d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| evaluateHandle<R, Arg, E extends SVGElement | HTMLElement = SVGElement | HTMLElement>(pageFunction: PageFunctionOn<E, Arg, R>, arg?: Arg, options?: { timeout?: number }): Promise<SmartHandle<R>>; | ||
| evaluateAll<R, Arg, E extends SVGElement | HTMLElement = SVGElement | HTMLElement>(pageFunction: PageFunctionOn<E[], Arg, R>, arg?: Arg): Promise<R>; | ||
| waitForFunction<Arg, E extends SVGElement | HTMLElement = SVGElement | HTMLElement>(pageFunction: PageFunctionOn<E, Arg, any>, arg?: Arg, options?: { timeout?: number }): Promise<void>; | ||
| waitForFunction<Arg, E extends SVGElement | HTMLElement = SVGElement | HTMLElement>(pageFunction: PageFunctionOn<E, Arg, any>, arg?: Arg, options?: { timeout?: number, signal?: AbortSignal }): Promise<void>; |
There was a problem hiding this comment.
What about other overrides here that have a timeout? I think these should get a signal as well.
There was a problem hiding this comment.
i think there's many more missing, I'll do those chunked in follow-ups.
Test results for "MCP"3 failed 7458 passed, 1132 skipped Merge workflow run. |
Test results for "tests 1"2 failed 2 flaky49217 passed, 1163 skipped Merge workflow run. |
Summary
signal?: AbortSignaloption next to every existingtimeoutoption on action, getter and navigation methods.__cancel__wire message that aborts the dispatcher'sProgressController; a pre-aborted signal throwssignal.reasonsynchronously.progressworks on the dispatcher side. That way the compiler enforces we forward it everywhere, rather than letting it hide in a params field.AbortSignalsupport to client-side Waiter methods #41136 (Waiter-basedwaitFor*methods), which is excluded here as it already has signal support.Not included (follow-ups)
expectassertions (toBeVisible, etc.) — deferred to a separate PR.ElementHandle.inputValue— intentionally left out. UnlikeLocator/Page/Frame.inputValue(which route through theFrame.inputValueprotocol method that carriestimeout), theElementHandle.inputValueprotocol method (packages/protocol/spec/handles.yml) declares no parameters, so thetimeoutoption the public types have long advertised there is silently dropped by the wire validator. Addingsignalwould require a signature change and inherit that broken-timeout behavior. Fixing it is a separate change: addtimeout/signalto theinputValueentry inhandles.yml.Refs #40578